Skip to content

call pageBuilder close function to release buffer#225

Open
case-k-git wants to merge 1 commit intoembulk:masterfrom
case-k-git:feature/call-pageBuilder-close-function
Open

call pageBuilder close function to release buffer#225
case-k-git wants to merge 1 commit intoembulk:masterfrom
case-k-git:feature/call-pageBuilder-close-function

Conversation

@case-k-git
Copy link

@case-k-git case-k-git commented Aug 25, 2021 •
edited
Loading

Summary

pageBuilder.close is not called and could be cause leak issue. add pageBuilder.close to to release buffer (buffer.release())

code

    @Override
    public void close() {
        this.delegate.close();
    }

https://github.com/embulk/embulk/blob/76a0c9922c551f1d7e6b964642a202edab745435/embulk-api/src/main/java/org/embulk/spi/PageBuilder.java#L201

    @Deprecated  // https://github.com/embulk/embulk/issues/1321
    public PageBuilder(BufferAllocator allocator, Schema schema, PageOutput output) {
        this(createImplInstance(allocator, schema, output));
    }

https://github.com/embulk/embulk/blob/76a0c9922c551f1d7e6b964642a202edab745435/embulk-api/src/main/java/org/embulk/spi/PageBuilder.java#L49

    private static PageBuilder createImplInstance(final BufferAllocator allocator, final Schema schema, final PageOutput output) {
        try {
            return Holder.CONSTRUCTOR.newInstance(allocator, schema, output);
        } catch (final IllegalAccessException | IllegalArgumentException | InstantiationException ex) {
            throw new LinkageError("[FATAL] org.embulk.spi.PageBuilderImpl is invalid.", ex);
        } catch (final InvocationTargetException ex) {
            throwCheckedForcibly(ex.getTargetException());
            return null;  // Should never reach.
        }
    }

https://github.com/embulk/embulk/blob/76a0c9922c551f1d7e6b964642a202edab745435/embulk-api/src/main/java/org/embulk/spi/PageBuilder.java#L204

    @Override
    public void close() {
        if (buffer != null) {
            buffer.release();
            buffer = null;
            bufferSlice = null;
        }
        output.close();
    }

https://github.com/embulk/embulk/blob/76a0c9922c551f1d7e6b964642a202edab745435/embulk-core/src/main/java/org/embulk/spi/PageBuilderImpl.java#L247

releated

embulk/embulk#1218 (comment)

@hiroyuki-sato
Copy link
Member

hiroyuki-sato commented Aug 25, 2021

Hello, @case-k-git.

Thank you for creating this PR.

We know that this problem has been unresolved for a long time.
Embulk Core team are investigating how to resolve this issue.
If this PR can fix the current problem, please describe it in detail.
Otherwise, please use a plugin as is.

@dmikurube
Copy link
Member

dmikurube commented Aug 26, 2021

Thanks for your contribution! But sorry that we have to say please wait for getting this merged.

To be honest, we're not yet 100% confident of :

  • Is this (plugin's side) the real root cause?
    • If this is the real cause, why it is not always happening, but only when handling so large data?
  • How other plugins implement this part?
  • How many plugins have this issue?
  • Would this be the "right" fix?
    • For example, modifying the core side could be a better fix, if many other plugins have the same issue.

At the same time, the team is now involved in the v0.10 efforts, then we have no resources to dive deeper into those questions.

@case-k-git
Copy link
Author

case-k-git commented Aug 27, 2021 •
edited
Loading

@hiroyuki-sato @dmikurube

Thank you for your reply and check.

I see . I will be wait until finish to Embulk Core team investigation finished

I am not sure if this is related.
previously I have got Buffer detected double release() when using sqlserver-input-plugin and output-bigquery-plugin.
this error has been caused also when transfering large amount of data.

and caused when disk space is not enough(by using enough space the error has been gone).
embulk/embulk-output-bigquery#138
seems to be resource is not handling well

I am not test it but if create small size of disk using docker in local env same error could be make without large amount of data.
using sqlserver-input-plugin and output-bigquery-plugin
(I am not sure output-elasticsearch-plugin)

hope this gonna help to investigate

thank you

@JPWon-1
Copy link

JPWon-1 commented Jul 8, 2022

I have same issue when i try to move data
from redis to bigquery

so like @case-k-git said,
it could be embulk core issue or
it could be embulk-output-bigquery issue

@JongpilW0N
Copy link

JongpilW0N commented Jul 12, 2022 •
edited
Loading

I have same issue when i try to move data from redis to bigquery

so like @case-k-git said, it could be embulk core issue or it could be embulk-output-bigquery issue

*redis to bigquery -> bigquery to redis
so it was not embulk-output-bigquery, but embulk-input-bigquery

anyway i think bigquery plugin has some issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@case-k-git @hiroyuki-sato @dmikurube @JPWon-1 @JongpilW0N

Comments